Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Merged by Bors] - fix bevy_ecs macro path handling #1426

Closed
wants to merge 1 commit into from
Closed

[Merged by Bors] - fix bevy_ecs macro path handling #1426

wants to merge 1 commit into from

Conversation

jakobhellermann
Copy link
Contributor

  • It now doesn't search in the dev-dependencies anymore
  • and the behaviour is consistent for derive_bundle and derive_system_param

@DJMcNab
Copy link
Member

DJMcNab commented Feb 10, 2021

How does this work if a user only uses the procedural macro in tests or examples?

@jakobhellermann
Copy link
Contributor Author

The use case where I ran into this was adding bevy to bevy_app's dev-dependencies, so that we can use bevy::prelude::*; in doc tests. The macro saw bevy in the dev-dependencies so tried to use that, but doctests don't have access to dev-dependencies.

The only way it would break now is if someone has bevy (or bevy_ecs) in their dev-dependencies but not in the main dependencies, which I think is pretty unlikely.

@jakobhellermann
Copy link
Contributor Author

But to support that we could first try in regular dependencies and afterwards the dev-dependencies. Should I do that?

@DJMcNab
Copy link
Member

DJMcNab commented Feb 10, 2021

I think so? I'm getting a massive sense of deja-vu - that is, I feel like we've already made this change somewhere else already, although I can't find it now.

But yes, trying in regular dependencies first and then dev dependencies does make sense.

@MinerSebas
Copy link
Contributor

MinerSebas commented Feb 10, 2021 •
edited
Loading

In #1182 there was a similar change made to the reflect crate, to check regular dependencies first, and dev-dependencies second.

@jakobhellermann
Copy link
Contributor Author

I changed it. I think build-dependencies can be ignored 😄

DJMcNab
DJMcNab approved these changes Feb 11, 2021
View reviewed changes
Copy link
Member

@DJMcNab DJMcNab left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. One small suggestion cart might want to merge, but equally this can also be merged as is.

crates/bevy_ecs/macros/src/lib.rs Outdated
};
let crate_path: Path = syn::parse(path_str.parse::<TokenStream>().unwrap()).unwrap();

let crate_path = bevy_ecs_path();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe ecs_crate or even just ecs would be better?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed it to ecs.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

imo removing "path" from the name makes it harder to tell what the variable is. can we do ecs_path?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or alternatively ecs_crate?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed it to ecs_path.

crates/bevy_ecs/macros/src/lib.rs Outdated Show resolved Hide resolved
@DJMcNab
Copy link
Member

DJMcNab commented Feb 11, 2021

Don't fix the clippy failures in this PR please - someone else will do that soon

@jakobhellermann jakobhellermann mentioned this pull request Feb 12, 2021
Closed
@woubuc
Copy link
Contributor

woubuc commented Feb 12, 2021

I don't know if you caught this but I added a couple of missing paths to the SystemParam macro in #1434 the other day. It looks like those are still missing in this PR.

@jakobhellermann
Copy link
Contributor Author

jakobhellermann commented Feb 12, 2021 •
edited
Loading

Thanks, fixed them now.

DJMcNab
DJMcNab approved these changes Feb 12, 2021
View reviewed changes
Copy link
Member

@DJMcNab DJMcNab left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything looks good

@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior A-ECS Entities, components, systems, and events S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it labels Feb 17, 2021
Base automatically changed from master to main February 19, 2021 20:44
@cart
Copy link
Member

cart commented Feb 22, 2021

resolve merge conflicts and I think this is good to go!

@jakobhellermann
fix bevy_ecs macro path handling
d6b5c3e 
- It now doesn't search in the dev-dependencies anymore
- and the behaviour is consistent for derive_bundle and derive_system_param
@jakobhellermann
Copy link
Contributor Author

done

@cart
Copy link
Member

cart commented Feb 22, 2021

bors r+

bors bot pushed a commit that referenced this pull request Feb 22, 2021
@jakobhellermann
fix bevy_ecs macro path handling (#1426)
a1ec684 
- It now doesn't search in the dev-dependencies anymore
- and the behaviour is consistent for derive_bundle and derive_system_param
@bors
Copy link
Contributor

bors bot commented Feb 22, 2021

Pull request successfully merged into main.

Build succeeded:

@bors bors bot changed the title fix bevy_ecs macro path handling [Merged by Bors] - fix bevy_ecs macro path handling Feb 22, 2021
@bors bors bot closed this Feb 22, 2021
bors bot pushed a commit that referenced this pull request Mar 3, 2021
@woubuc
Add docs & example for SystemParam (#1435)
000dd4c 
It took me a little while to figure out how to use the `SystemParam` derive macro to easily create my own params. So I figured I'd add some docs and an example with what I learned.

- Fixed a bug in the `SystemParam` derive macro where it didn't detect the correct crate name when used in an example (no longer relevant, replaced by #1426 - see further)
- Added some doc comments and a short example code block in the docs for the `SystemParam` trait
- Added a more complete example with explanatory comments in examples
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cart cart cart approved these changes

@DJMcNab DJMcNab DJMcNab approved these changes

Assignees
No one assigned
Labels
A-ECS Entities, components, systems, and events C-Bug An unexpected or incorrect behavior S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@jakobhellermann @DJMcNab @MinerSebas @woubuc @cart @alice-i-cecile